Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial attempt at win32 http_listener response refactor #16

Merged
merged 8 commits into from
Dec 7, 2015

Conversation

gigaplex
Copy link
Contributor

For Issue 13: #13

Here is a first cut at a refactor for the response handling to reduce cyclic reference leaks and allow error replies to propagate back to the client. I'm not 100% happy with it, the content_ready_task for example will potentially race against async_process_response if they both attempt to call cancel_request under certain error conditions. There's also no attempt to catch exceptions on the proxy_content_ready completion event, which might cause some unobserved exception issues. I've created the pull request at this early stage to facilitate discussion.

@msftclas
Copy link

Hi @gigaplex, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@gigaplex
Copy link
Contributor Author

CLA is awaiting signature from our legal team, they've informally granted permission at this stage.

@ras0219-msft
Copy link
Contributor

To improve readability, could you use an enum for the bad_request argument of do_response instead of a bool? The meaning of do_response(response::GOOD_REQUEST) is more obvious compared do_response(false).

As mentioned in the other thread, I'm concerned about race conditions regarding the reading and setting of m_msg. If there are possible concurrent accesses, I think every reader will need to "pin" it to the stack by making a local copy of the shared_ptr so that it doesn't get deleted while they are inside a call. Example:

  • To start, m_msg is the only strong ref to the actual request object.
  • Thread 1 calls m_msg.reply() (link).
  • After the pointer load completes on Thread 1, Thread 2 calls m_msg = http_request() (link). This removes the last remaining strong ref to the actual request object, so it is deleted.
  • Thread 1 proceeds to modify the now-freed request object, invoking UB of the worst sort.

@gigaplex
Copy link
Contributor Author

Thanks for the initial feedback. I chose to use a bool instead of an enum because that's how the asio code currently does it, but I agree, I usually prefer enums here.

As for the race condition in your example, m_msg isn't the only strong ref to the request object. There's a local copy in the argument to async_process_request. Also, there's a reason that do_response isn't called until after m_msg.reply() completes, so thread 2 can't call m_msg = http_request() because that callback isn't initialised until after the reply is created. I have however found a race condition of a similar nature in another function, which I'm working to address.

@gigaplex
Copy link
Contributor Author

I submitted the CLA a few days ago, yet this is still tagged as cla-required. How long does it normally take to process?

@kavyako
Copy link
Contributor

kavyako commented Nov 18, 2015

yes, this seems to be taking unusually long, I am following up on this with our CLA automation team.

@kavyako kavyako closed this Nov 18, 2015
@kavyako kavyako reopened this Nov 18, 2015
@msftclas
Copy link

Hi @gigaplex, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@gigaplex
Copy link
Contributor Author

I'm fairly happy with the current state of my changes now. Can I get another round of review for merging please?

One question I do have is that in the m_response_completed callback, immediately after p_context->m_response._get_impl()->_set_server_context(nullptr); there was a call to t.get();. What is the purpose of this call? It wasn't carried over in my refactor, but if it's needed for a specific reason, I can add it back in.

@gigaplex
Copy link
Contributor Author

I've rebased against 2.7.0.

@ras0219-msft ras0219-msft merged commit 6c82566 into microsoft:development Dec 7, 2015
@ras0219-msft
Copy link
Contributor

Sorry for the delay on this one, thank you very much for your patience and the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants